Conversation
|
Thanks for the focused recovery hardening. The reported failure remains present on current Verified current-main behavior
This is an automated hermes-sweeper review. |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Eighteen PRs address or reference this Weixin reliability cluster, covering several distinct causes: stale context_token variants (unknown error, empty, rate limited, and prepare failed), tokenless-retry mechanics, genuine rate-limit handling, long-poll recovery, and ret=-3. For the target PR, #74572 specifically fixes outbound text sends returning ret=-2, errmsg="prepare failed" and hardens token deletion and retry accounting; it does not replace the narrower variant work in #35714 or the separate polling and genuine-throttling fixes.
Related pull requests
- #17287 [closed]
fixes— (+15/-1) — incorporated reference: introduced the sharedret=-2, errmsg="unknown error"stale-session classifier and connected it to polling and outbound sends. Although closed, it remains relevant because its commit was cherry-picked with preserved authorship into #17432 ase9b96fd05. - #17432 [merged]
best fix— (+45/-1) — merged reference implementation: landed #17287'sunknown errorclassifier and seven truth-table tests, establishing the tokenless-recovery baseline. It does not cover empty,rate limited, orprepare failedresponses. - #18105
partial— (+16/-7) — keep open with a salvage path: its empty/None-errmsgclassifier addresses #18100, but the keep_open review requires normalizing the alternatemsgfield and adding outbound tokenless-retry and genuine-rate-limit regressions so a populatedmsg="frequency limit"is not misclassified. - #20797 [closed]
partial— (+27/-8) — duplicate of #18105: it makes the same empty/None-errmsghelper change and equivalent classification tests. It remains relevant only as closed corroborating evidence because its author explicitly closed it in favor of #18105. - #22661
partial— (+34/-1709) — keep open only for focused salvage: the Weixin portion broadens stale-session matching and attempts token stripping, but the diff also deletes twelve unrelated workflows, lacks matching tests, and places fallback after a circuit-breaker point that can suppress it. This follows the maintainer-bot keep_open verdict: retain only the reordered, tested Weixin logic on current main. - #23529
partial— (+27/-11) — keep open with a broader reconnect salvage path: propagatingasyncio.TimeoutErrorexposes the zombie-poll symptom, but the keep_open review notes that the generic poll handler still neither reconnects nor transitions health state. The fix needs a failure-threshold lifecycle change and replacement of the current contradictory sentinel test. - #23534 [closed]
related— (+1/-1) — rejected polling approach: it converts every long-poll timeout intoret=-3, while the blocking contributor decision documents such idle timeouts as normal and warns that this would produce false failures and backoff. It remains relevant as a closed design boundary, not as a recovery implementation. - #27234 [closed]
best fix— (+8/-3) — deferred genuine-rate-limit experiment: replaces fixed waits with a long exponential schedule and raises the retry count, but does not correct stale-token classification. It remains relevant after closure only as an author-deferred backoff alternative overlapping #31132. - #31132
best fix— (+59/-10) — keep open with a current-main salvage path: adds retry hints, jittered exponential backoff, and chat cooldown for genuine throttling, but the keep_open review shows the default circuit opens before that wait can run; the diff also removes attachment path filters and adds untested configuration. Reconcile the circuit, retain path filtering, and add deterministic tests. - #34216 [closed]
partial— (+4/-2) — duplicate of #23529: it converts client timeout into synthetic-999and also changes empty-errmsgclassification, but provides no actual poll-session rebuild. It remains relevant as a closed competing timeout approach, not as a complete fix for #23523. - #35066
best fix— (+65/-1) — keep open for the one-line guard salvage: removingand context_tokenpermits one tokenless recovery attempt when no cached token exists. The keep_open review requires replacing the invalidret=-3fixture with a recognizedret=-2, errmsg="unknown error"orerrcode=-14case, and field testing says this alone does not solve the platform-levelret=-3failure. - #35283 [closed]
best fix— (+2927/-29) — closed mixed superset requiring focused splits: it contains potentially useful timeout/reconnect and typing-cadence work, but its final send code contradicts its empty-errmsgclassifier and tests, adds noncompliant behavioral environment knobs, and conflicts with the current circuit. Despite the earlier keep_open review, the author accepted those findings and closed it to split the salvage into focused PRs. - #35666 [closed]
best fix— (+45/-34) — closed mixedret=-3experiment: adds-3stale-session handling and removes the no-token retry guard, but also changes unrelated launchd and gateway self-stop protections, while field evidence says tokenless retry did not resolve the observedret=-3platform behavior. Preserve theret=-3evidence for a focused split rather than reviving this diff. - #35714
best fix— (+38/-8) — keep open with a focused classifier salvage path: recognizes normalizedrate limited, empty/None, andunknown errorvariants while retainingfreq limitas genuine throttling, with focused tests. Its maintainer-bot keep_open review reports no blocking static issue, but it does not include #74572's compare-and-delete, under-lock token load, or retry-budget hardening. - #35983
partial— (+20/-5) — close as duplicate of #35714 after preserving its send/poll test idea: both addrate limitedto stale-token classification, but this diff changes the shared helper without the requested path-level tests and can route polling into a 600-second session-expiry sleep. Despite the keep_open review on #35983, the diff shows that its stated send-only retry guard does not protect the shared polling call site. - #62386
partial— (+45/-5) — close as duplicate of #35714: it reaches tokenless recovery by treating every token-bearing-2as stale, including documented genuine frequency-limit responses, and alters circuit tests to remove their tokens. Despite the keep_open review on #62386, that concrete overbroad predicate is weaker than #35714's message discriminator; retain only useful context-token path tests. - #73908 [closed]
fixes— (+63/-13) — closed overlapping empty-errmsg implementation: adds durable token deletion and an end-to-end tokenless retry test, but duplicates the empty/None coverage now present in #35714. Its persistence work remains useful evidence, although #74572 improves it with compare-and-delete so a concurrently refreshed token is preserved. - #74572
partial— (+222/-18) — keep open with a salvage path: adds an outbound-onlyprepare failedclassifier, compare-and-delete persistence, token loading under the outbound gate, a recovery attempt outside the normal retry budget, and continued tokenless retries. This matches its high-salvageability keep_open verdict and directly fixes the reported target cause without changing polling, media, or genuine rate-limit behavior.
Duplicates
#17287 was incorporated into #17432. #20797 duplicates #18105, and closed #73908 overlaps their empty/None-errmsg change while contributing persistence tests. #35983 and #62386 target the same ret=-2, errmsg="rate limited" stale-token failure as the earlier #35714, but #35983 leaks the classification into polling and #62386 overgeneralizes every token-bearing -2. #34216 overlaps #23529 on zombie polling; #27234 overlaps #31132 on genuine-rate-limit backoff. #35283 bundles pieces of several groups but was closed for focused rework. #74572 is not a duplicate of #35714: it covers the outbound-only prepare failed variant and stronger recovery mechanics.
Suggested consolidation
Keep #74572 open with a salvage path: retain its outbound-only prepare failed classifier, compare-and-delete token persistence, under-gate token load, and explicit tokenless recovery attempt, and keep its path-level regressions. Keep #35714 as the focused home for the distinct normalized rate limited and empty/None variants, while adding an outbound tokenless-retry regression and confirming that polling behavior remains intentional. Close #35983 as duplicate of #35714 despite its keep_open review because its shared-helper diff exposes polling to the new classification; close #62386 as duplicate of #35714 despite its keep_open review because treating every token-bearing -2 as stale bypasses genuine frequency-limit handling. Leave #18105, #22661, #23529, #31132, and #35066 open only with the concrete review-requested salvage actions above; the remaining closed PRs should stay closed as incorporated references, duplicates, rejected designs, or sources for focused future splits.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I62383(["issue #62383 (open)"])
P74572["PR #74572 (open)"]
P74572 -.->|partial| I62383
class I62383 open
class P74572 open
class P74572 target
click I62383 "https://github.com/NousResearch/hermes-agent/issues/62383"
click P74572 "https://github.com/NousResearch/hermes-agent/pull/74572"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).
Cross-PR triage: Reviewed 18 pull requests and 9 issues in this complex. Each diff was read against this issue; Assessment working set: 277 kB of PR diffs, 67 kB of issue/PR text, 38 kB of discussion (51 comments), 49 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
abcfb33 to
e829fad
Compare
|
|
What does this PR do?
Recovers scheduled Weixin text delivery when iLink reports a stale
context_tokenasret=-2witherrmsg="prepare failed".The fix is intentionally limited to the text
sendmessagepath. It does not change media delivery.The adapter previously recognized only the
unknown errorstale-session variant and otherwise treatedprepare failedas rate limiting. The initial fix also left edge cases where recursive recovery could reset the retry budget, reuse the original stale token after a tokenless failure, delete a newer token received concurrently, or read a token before the outbound lock was acquired.This revision:
prepare failedonly for outbound text context-token recovery;getUpdates, media delivery, and genuine rate-limit behavior unchanged.Related Issue
Follow-up to #17228.
Type of Change
Changes Made
gateway/platforms/weixin.py: add outbound-only stale-token classification, compare-and-delete token invalidation, lock-time token lookup, and explicit bounded retry state for text sends.tests/gateway/test_weixin.py: cover zero-retry recovery, bounded failure after stale recovery, concurrent fresh-token preservation, lock-time lookup, persistence/account isolation, and multi-chunk behavior.How to Test
scripts/run_tests.sh tests/gateway/test_weixin.py -q.python -m ruff check gateway/platforms/weixin.py tests/gateway/test_weixin.py.python scripts/check-windows-footguns.py gateway/platforms/weixin.py tests/gateway/test_weixin.py.git diff --check.Expected result: all 37 Weixin tests pass, lint passes, no Windows footguns are reported, and the patch has no whitespace errors.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) - N/A; no user-facing API or configuration changecli-config.yaml.exampleif I added/changed config keys - N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows - N/AScreenshots / Logs
Follow-up verification:
git diff --check: passed.A repository-wide suite was attempted earlier in this native Windows checkout: 21,821 tests passed and 462 failed outside the touched Weixin test file, predominantly around native-Windows path/permission assumptions or unavailable optional dependencies. The scoped Weixin suite was green in that run; GitHub CI remains the authoritative full-suite result.